-
Notifications
You must be signed in to change notification settings - Fork 156
Consolidate time series downsampling docs #2274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
…s-content into mw-tsds-downsampling
This comment was marked as outdated.
This comment was marked as outdated.
manage-data/data-store/data-streams/downsampling-time-series-data-stream.md
Outdated
Show resolved
Hide resolved
manage-data/data-store/data-streams/downsampling-time-series-data-stream.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @marciw, this revamp looks really great! I have added some small comments and pinged Mary to help with an additional review / pair of eyes - her expertise on downsampling will be invaluable to iron out the last few details and move forward!
Co-authored-by: Yannis Roussos <[email protected]>
Co-authored-by: Yannis Roussos <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
@yannis-roussos @gmarouli I've addressed most of your comments and asked for clarification on a few others. Please take another look when you have time. 🙏 Also note that this is just one piece of the overall restructuring -- additional PRs coming soon |
for easy access, here's the link to the preview page -- you can start here and use the "next" link at the bottom of each page https://docs-v3-preview.elastic.dev/elastic/docs-content/pull/2274/manage-data/data-store/data-streams/downsampling-time-series-data-stream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @marciw , I added some small comments but I am very happy with how it looks, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @marciw, looks great! I agree with Mary that this looks ready for prime time!
Co-authored-by: Mary Gouseti <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚢
This looks great @marciw!
Co-authored-by: David Kilfoyle <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like SMEs are happy, changes LGTM 👍.
I just wonder if we should pay attention to the H1s and add additional context 🤷.
For example, should we have stuff like :
- Downsampling concepts
+ How downsampling in {{es}} works
🙏 yes! coming soon in a follow-up PR (I did these in phases and I think that ended up being confusing) |
This PR restructures and partially revises the downsampling section:
closes #2239
TODO: Create issue for "fast follow" improvements, in the interest of time